Basic Multi-User Setup #217
Conversation
|
References Issue #24 |
There was a problem hiding this comment.
Basic Multi-User Setup commit
Marked an error (I think).
Add Jellyfin "Enable" toggle
Marked a missing json_decode.
get to the rest later this week
Please take as much time as you need.
This may be annoying for users that are currently using Jellyfin (they'd probably have to go re-enable it). Not sure if you have any specific way you want to solve that?
I'll just leave a note in the update notes.
Middleware for admin sections
I'll check and move every route to the admin group when the PR is ready otherwise.
Lastly, I have no idea how the anki stuff works (haven't looked at all yet) - I've not used this feature personally. Let me know if there's anything I should know for that (or if someone else wants to take over that part)
It basically just sends AnkiConnect plugin http requests to add review cards to it. Anki is a review software desktop application.
It might be a bit difficult to understand if you are not familiar with Anki. If we release multi user feature without rewriting this part everything would work, except if a user clicks on the send to Anki button, it would get an error. It could cause some confusion.
I could rewrite it in a few hours, but honestly I'm feeling a bit burned out, I want to focus on other things for a while.
Can I ask how bad or good my code is to work on? I rarely worked with other people before, so I never got any feedback on it.
|
Great thanks, made those changes. Code is all clear and easy to work on! Main thing I noticed though is that all api routes are in the web routes file. Not an issue but when creating middleware that returns a response we have to be careful whether it's a JSON or HTML response based on which endpoints it's used on. Splitting up the routes makes it easier to reason about that. I have used Anki before - I will go ahead doing the Anki stuff if there's no rush, after I set it up on my live deployment which I should probably do anyway :) |
Currently refactoring my controllers #103. In the files that are already complete, they (should) all return JSON responses, except the Home controller's index which is the actual page.
I reread my first description, I don't think I explained it well. The problem is that currently when a user sends a card to Anki, the browser sends a request to the server, and the server sends it to Anki. But if there are multiple users, it doesn't make sense, so instead their own browser should send the Anki requests to their own Anki program. But this way the logic must also move from the server to the client. Actually, just realized that there might be a problem. If we make this change, the client will have to have access to the Anki program's network. So if someone hosts linguacafe, they won't be able to send cards to Anki remotely through the linguacafe server, the client itself will have to have acces to Anki's network. I think this is an acceptable compromise, but not sure. |
|
Hi! I will release v0.13 sometime in June, probably mid-end June. After that I will update to Laravel 11, which would make this opened PR very hard to manage. If you would like to, we could cut multi user feature into multiple parts. We can finish disabling Jellyfin (on the import UI), and I can add the admin routes to the middleware, then do the rest later after it was migrated to Laravel 11. I do not want to hurry you, I just want to figure out what to do to avoid conflicts for opened PR-s. If you would like, I can also finish the rest of the feature. Thank you for working on this, I really appreciate all contributions! And I hope you enjoy LinguaCafe! |
|
Hey - yeah sorry I don't have much time to work on this as stuff has come up. If you can finish it that would be great. Let me know if you have any questions about my changes though! |
That's completely okay, I'l finish it. |
|
Well. That was the hardest conflict resolution of my life. In the end I made it work, but has a lot of extra commits now. Anyways, thank you so much for the PR, I really appreciate it! I'll finish disabling Jellyfin on the UI, and move the admin routes into the middleware. I'll add a warning about the rest of the problems, they can be fixed later. |
This is not ready to merge. I haven't quite finished/tested properly yet and will get to the rest later this week. But I thought I'd get this pull request up so you can take a look and let me know if you want anything done differently.
Main things are:
Added an endpoint to check if the current user is an admin.
This might have been better done as part of the general 'get user data' endpoint and then we could just check the is_admin property specifically whenever needed. However I see there's no global state and didn't want to pass data between components too much. This endpoint is currently only used on the admin settings page (in case the user navigates there manually it will just show an error).
Jellyfin default enabled setting
Currently the jellyfin 'enabled' toggle defaults to false. This may be annoying for users that are currently using Jellyfin (they'd probably have to go re-enable it). Not sure if you have any specific way you want to solve that? Of course we could just default it to true, but seems odd to be enabled when they haven't added their api key etc.
Middleware for admin sections
I've added a middleware to protect the admin sections. There are various other endpoints that should be moved into that middleware section - but I know I'd miss plenty so figured perhaps better if someone more familiar with all the routes did this.
Lastly, I have no idea how the anki stuff works (haven't looked at all yet) - I've not used this feature personally. Let me know if there's anything I should know for that (or if someone else wants to take over that part)